Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: manipulation of node structures, proper stringifying #73

Merged
merged 3 commits into from Mar 7, 2019

Conversation

shellscape
Copy link
Owner

@shellscape shellscape commented Mar 6, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

If yes, please describe the breakage.

Please Describe Your Changes

This PR fixes #69, wherein manipulating the child nodes for a particular node (in this case a Func node) did not reflect in stringifying of the node. This solution was derived from the following test code:

const { nodeToString, parse } = require('./lib');
const Punctuation = require('./lib/nodes/Punctuation');

const root = parse('rgb(100% 100% 100%)');

root.first.nodes.splice(1, 0, new Punctuation({ value: ',', parent: root.first }));
root.first.nodes.splice(3, 0, new Punctuation({ value: ',', parent: root.first }));

console.log(nodeToString(root));

The result of which is now:

rgb(100%, 100%, 100%)

Note that the parent property is being set in the code above, whereas the referenced issue did not set the property. This is required for PostCSS internals to accurately determine raws values to render.

Since we were previously only rendering the params property of functions (which is a string reference to unparsed function parameters), some additional work had to be performed to correctly capture and render spacing around and within the function parens. I believe this PR covers all of the various cases we may encounter.

@jonathantneal
Copy link
Collaborator

Reviewed the code and the snapshot. The parent requirement makes sense. Thanks!

Copy link
Collaborator

@jonathantneal jonathantneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t see how params is handled here. Should there be a test to demonstrate that it is updated by nodes and/or vice versa?

@shellscape
Copy link
Owner Author

I don’t see how params is handled here

Are you speaking of the params property? If so, that property shouldn't be updated. It's a snapshot of what the function params were in the source. The documentation should be updated as well though, so that's a good catch.

@jonathantneal
Copy link
Collaborator

Sounds good to me, then. Thanks!

Copy link
Collaborator

@jwilsson jwilsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shellscape shellscape merged commit bc08be3 into master Mar 7, 2019
@shellscape shellscape deleted the fix/manipulation branch March 7, 2019 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stringifier Ignores Manually Added Func Child Nodes
3 participants